Skip to content
This repository has been archived by the owner on May 14, 2024. It is now read-only.

Support Node.js 10 #497

Merged
merged 2 commits into from
Aug 27, 2019
Merged

Support Node.js 10 #497

merged 2 commits into from
Aug 27, 2019

Conversation

acappella2017
Copy link
Contributor

@acappella2017 acappella2017 commented Nov 27, 2018 •
edited
Loading

We observed some weird behaviors with node-ldapjs when upgraded to Node.js 10 LTS in our project.

This issue is caused by some internal change in Node.js. See nodejs/node#24577.

In summary, in Node.js 10, the close event depends on an internal end listener. so when ldap client use removeAllListeners(end`) to do cleanup work, the close listener stops working.

Node.js guys suggest that we should use removeListener to explicitly remove the listener we want to remove instead of using removeAllListeners. Node.js document also suggests that we should use removeAllListeners carefully.

Note that it is bad practice to remove listeners added elsewhere in the code, 
particularly when the EventEmitter instance was created by some other component or 
module (e.g. sockets or file streams).

This PR didn't touch all of the removeAllListeners in client.js, just fixed the one who breaks close event.

@jgeurts, @melloc, could you help to review this PR?

This was referenced Nov 28, 2018
Closed
Closed
@sunpeinju
Copy link

@melloc @pfmooney
Could you help to take a look?

@acappella2017 has found the root cause (removeAllListeners even removes TLS socket's internal listeners and breaks socket's event sequence) and discussed with Node team. And this PR contains the safest change we can find, it doesn't change current code logic.

Many projects and npm modules depending on ldapjs need it to work on Node.js V10. Hope to see this PR get reviewed.

@longuniquename
Copy link

Hi. Any progress on this?

@sarus sarus mentioned this pull request Jan 5, 2019
Closed
kspearrin added a commit to kspearrin/node-ldapjs that referenced this pull request Mar 22, 2019
@kspearrin
Support Node.js 10 ldapjs#497
12c07b1
@kspearrin
Copy link

I just forked the repo and am using my own version with this fix in place.

"ldapjs": "git+https://git@github.com/kspearrin/node-ldapjs.git",

jharris4 added a commit to jharris4/node-ldapjs-jh that referenced this pull request Apr 3, 2019
@jharris4
add node 10 fix
5f1d4a9 
ldapjs#497
@zachlendon
Copy link

Hi!

Any updates/plans on this? It seems multiple individuals are left to fork to get this fix into their projects, is it mergeable here or is non-review work outstanding?

@nickzelei
Copy link

@zachlendon This project is pretty much dead. None of the maintainers have commented on here in years.

@jharris4
Copy link

jharris4 commented Jun 5, 2019

For what it's worth, we started having major issues with ldap using a verdaccio server with the verdaccio-ldap plugin after upgrading to Node 10. The verdaccio ldap plugin uses this package.

I tried using the fork of this project to resolve the issues without any luck, so we had no choice but to downgrade to Node 8.

@jsumners jsumners mentioned this pull request Jun 15, 2019
Closed
@xPaw xPaw mentioned this pull request Jul 10, 2019
Merged
1 task
@lneveu lneveu mentioned this pull request Jul 11, 2019
Merged
@bryancuster
Copy link

Any updates or reasons to not merge this?

@jsumners
Copy link
Member

@acappella2017 would you be able to add a unit test for this?

@acappella2017
Copy link
Contributor Author

@acappella2017 would you be able to add a unit test for this?

@jsumners Without this pr, ldapjs UT hangs with node 10. I believe current UTs are able to cover this change.

@jsumners jsumners changed the base branch from master to next August 27, 2019 12:09
jsumners
jsumners approved these changes Aug 27, 2019
View reviewed changes
Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coverage remains the same before and after this PR. So I'll assume this is covered by any unbind tests that currently exists. We need a test coverage audit in the future anyway.

@jsumners jsumners merged commit a8b09f3 into ldapjs:next Aug 27, 2019
@jsumners jsumners mentioned this pull request Aug 27, 2019
Closed
@scottbell scottbell mentioned this pull request Sep 19, 2019
Closed
AnteroMediSapiens pushed a commit to Medisapiens/node-ldapjs that referenced this pull request Feb 11, 2020
@acappella2017
Support Node.js 10 (ldapjs#497)
012afee 
* Support node 10

* remove arrow function
pcworld added a commit to pcworld/node-ldapauth-fork that referenced this pull request Nov 25, 2020
@pcworld
README: Remove outdated comment about Node v10
016162c 
The mentioned issue has been resolved by ldapjs/node-ldapjs#497
@pcworld pcworld mentioned this pull request Nov 25, 2020
Merged
@differsthecat differsthecat mentioned this pull request Jan 19, 2022
Merged
9 tasks
@jsumners
Copy link
Member

⚠️ This issue has been locked due to age. If you have encountered a recent
problem that seems to be covered by this issue, please open a new issue.

Please include a minimal reproducible example
when opening a new issue.

@ldapjs ldapjs locked as resolved and limited conversation to collaborators Mar 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jsumners jsumners jsumners approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@acappella2017 @sunpeinju @longuniquename @kspearrin @zachlendon @nickzelei @jharris4 @bryancuster @jsumners